Is it worth converting this to an enum while we're here?

Should we also add a new value to p1 called e.g. SET_FROM_BITMASK such that we could combine the above into:

I quite like that idea. It is more inline with how I think of bitmasks. Also it ompletely separates the old behaviour from the new behaviour so the bitmasks are only used with that new param 1.
We'd have to check no one is doing something annoying like treating any non-zero value of param 1 as enabled.

Is it worth converting this to an enum while we're here?

Probably. If we do this it makes it easier to more clearly deprecate particular options, such as param1 option "2" over time.

But let's see what @andyp1per and others think.

I like the idea, but I think you need both - otherwise you need to make a call to find out what the current bitmask is.
So I think setting enable/disable with a bitmask should still be valid and then a new option which overwrites everything

I'm not sure I understand this ^^^. Why do you need to know the current bitmask? Whatever you set will become the enabled/disabled state with the new approach?

The other thing I like about using a bitmask in this way is that we could make it a proper bitmask - ie. not use 0 to mean "set all" but instead set nothing.

Lets say I only want to change the max altitude fence. I need to know whether any of the other fences are enabled because I want to preserve their state - I have to find that out from somewhere or stash it. With the enable/disable thing I don't need to remember this because I know I am only changing one specific fence - which TBH is the whole point.

@andyp1per Doh - thanks.

So to be clear, the only use case you really need here is to be able to disable the altitude geofence on takeoff, enable it again when flying, then disable it for landing?

If so, why can't you:

• Before takeoff send command with param1 = 2 - disables the floor check so you can take off
• When flying send command with param=1 - enable all
• Before landing send command with param1 = 2 - disables the floor check

I appreciate this new approach is more flexible but I don't see why you'd ever want to selectively turn on/off the other case.

Sorry if I'm being dumb here. We can skype if you like - hamishgw

Ultimately I want this to be finely controlled from scripting. Individually enabling and disabling notches is a requirement here. We also have tests that test individual fences in the air - I don't want to break that behaviour.

"I want to because I have requirements and tests" doesn't sound like a use case - it is easy to create a bogus requirement and then test against it". Can you give me one real case where you're flying and you want to disable all circle fences but not all polygon fences, and visa versa? I'm asking because the change should add value.

I am at a race track. I have a polygon fence around the track and a circle fence around the landing area. I want the polygon fence to always be active but I want the landing area to be deactivated on a switch when I am ready to come in to land.

That's pretty inflexible. I only have to add one more fence of any kind to mean that this no longer works.

As an aside, what's your understanding the conditions under which the fences revert to their system defaults (presumably "all enabled").
For missions I think it is pretty straightforward - generally settings in missions should revert to system defaults when the mode changes out of mission mode - although this is problematic for for flight stacks that implement mission pause as a mode change, since the current setting would be lost.

I'm not sure about what happens if this is sent in a command, and the docs don't say.

• It could be that when sent as a command this permanently changes the system default - i.e. it turns off the fences even if defined in the mission. If so, we'd need a way to know whether they are on or off.
• It could be that when sent as a command the change is temporary for the current mode. So (thinking in PX4 as more familiar for me) you'd set takeoff mode, send command to disable the altitude fence, arm and takeoff. The fence would then turn on as you switch from takeoff to hold. Or similar.
• Something else.

One of the things I am trying to fix is that mavlink currently makes permanent changes to the vehicle defaults - I want to support not doing this. My design assumption was that with this, whatever was sent from mavlink should override whatever is currently configured. We also have parameters relating to auto-enablement that are not reflected in mavlink - not permanently updating makes this a bit easier.

I can see benefits to either approach - setting the system defaults or a temporary change. It's not my call which is used, and I don't "care" provided the behaviour is specified, and the specification does not immediately make either ArduPilot or PX4 non-compliant (as that needs to be agreed/managed).

Assuming all are OK with this this case the description might change from:

<description>Mission command to enable the geofence</description>

to

<description>
  Enable the geofence.
  This can be used in a mission or via the command protocol.
  The effects persist while the mission is being executed, until superseded by another mission item or the mission completes.
  When used as a command (outside of mission) the value persists until superseded by another command, a mode change (which reverts system back to system defaults), or reboot.
  When used as a command while a mission is running the value is treated as though it were a mission item, and will change the current mission value.</description>

The points of interest being that:

• mission settings persist in the mission context, even on flight stacks that pause by switching mode to "Hold": on return to the mission the mission setting would be used.
• As a command the value only persists until mode change or reboot.
• A command can affect a mission

All of that is arguable, but IMO should be the default thinking for commands used in both missions and command protocol.

If you think this is reasonable I'll create an upstream PR to discuss.

Seems reasonable to me

@andyp1per For discussion, there is another way that this can be done, which might be better - two enums, one being a bitmask that says what you're enabling.

This allows you to specify any combination of flags to be enabled/disabled. It means, for example, that you don't have do know whether a particular fence type is enabled or disabled to start with, you can just use the second bitmask to specify that you're only interested in affecting the altitude floor fence (say).

We'd enable this completely separate of the original behaviour using param1=4

So something like:

Tomato, tomato - I think my way is better, but as long as I can get the functionality I am after - enabling and disabling of individual fences I don't really care. I'm not quite clear what happens to current calls with your scheme and the default values - does it really work?

It should.

I'm not quite clear what happens to current calls with your scheme and the default values - does it really work?

I'm not certain what you mean, but the new stuff would all be conditional on param1 = 4. Existing systems should reject a command with this value.